-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start adding a global event loop #57
Conversation
Thanks for this. I took away from the RFC discussion that it was not clear whether or not implicitly starting the global loop on first use would be better than explicitly starting the global event loop. Specific questions.
I think this PR should be continued to explore this space so these questions can be better addressed. I will add some more specific feedback inline. |
src/reactor/mod.rs
Outdated
} | ||
|
||
#[derive(Clone)] | ||
enum HandleRepr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this enum should be avoidable if you can create a HelperThread
instance without actually starting the thread. You would get the Handle
of the reactor that hasn't actually started running. If creating a reactor fails, you could set inner
to Weak::new()
.
Either way, I think being able to get the handle to the global reactor when the thread isn't running is useful in its own right. This goes back to the questions I posed in the above comment.
@carllerche Thanks for laying out those issues. As I expressed when we chatted about this on IRC, I would strongly prefer that Tokio by default lazily spin up a reactor thread, and provide an explicit opt out if you want to avoid this (which is the design in the RFC). This is both to make the defaults match the most common use case/best practices, but also to make it feasible to wrap Tokio-based libraries with synchronous APIs, while keeping Tokio a purely private dependency (i.e., not requiring the app to know about or initialize a Tokio event loop). I do believe this is purely a question of defaults, and that we should provide the full range of functionality you spell out. That said, it seems feasible and helpful to review the PR as-is, on the basis of the RFC text, and leave the questions you raise open in the tracker for follow up PRs, so that we don't get too bogged down at this juncture. |
@aturon Well, the RFC doesn't address the questions, so if we need to land things in the RFC first before PRs, maybe we should take this discussion back to the RFC and get that updated (I'm not entirely sure what you are getting at). |
The RFC specifies that an event loop thread will be started up automatically, and allows for using |
I guess I should clarify that (as I tried to say on IRC), I don't necessarily agree with your assessment.
libs could obviously explicitly start the global reactor as well, making tokio invisible to the end user. I could be convinced that implicitly starting the global event loop is the right option, but doing so would require answers to the questions I listed. If the right venue to hash those out is the RFC & up front design, then we can do that. I just thought that maybe, if the impl was provided in a PR, it would resolve the questions in my mind. |
@carllerche OK cool -- sounds like we agree on some core constraints:
You're also suggesting the following additional requirements:
I'm a little iffy on this constraint -- personally, I'm happy with the story that you can either use the default loop which runs forever once started, or else you fully manage it yourself. Can you spell out your thinking here a bit more? I didn't mean to lawyer about the RFC and whatnot. We can just use this PR to hash out these questions. |
Talked with @carllerche on IRC, and we came up with one plausible approach (which I think is a small delta over this PR):
You can think of the fallback reactor as a global, one-shot, threadsafe @alexcrichton would you mind making a commit adding this fallback reactor concept? @carllerche believes this is a plausible approach, but seeing the impl would probably help. |
} | ||
|
||
impl Default for Handle { | ||
fn default() -> Handle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the default handle point to the global reactor? For new users this might be bit confusing, and for users not using a global event loop this might point to the wrong event loop.
Would impl Handle { fn global() -> Handle }
(so we can call Handle::global
) be a better approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this was debated at some length on the RFC, but eventually this may not actually return a global handle in the sense that you'll be able to override the return value of Handle::default
within a particular program scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
This commit starts to add support for a global event loop by adding a `Handle::default` method and implementing it. Currently the support is quite rudimentary and doesn't support features such as shutdown, overriding the return value of `Handle::default`, etc. Those will come as future commits.
b2d50c5
to
0ab2013
Compare
@aturon that sounds reasonable! I've pushed up a commit which adds a |
@alexcrichton Yep, that is precisely what I had in mind! |
Ok, this looks like a good step to me. Two questions that don't necessarily have to be resolved this PR:
However, as I said, this can be moved to an issue and tracked separately as I don't think it is a core aspect of the "default reactor" design. Feel free to address or merge, either way. |
Ooops, messed up the merge, but it is here: 32f2750 |
This commit starts to add support for a global event loop by adding a
Handle::default
method and implementing it. Currently the support is quiterudimentary and doesn't support features such as shutdown, overriding the return
value of
Handle::default
, etc. Those will come as future commits.This PR is based on #56, only the last commit needs to be reviewed.